Implementing Firebase SDK Initialization and Custom Authentication #1

hiranya911 · 2017-03-20T20:57:10Z

First iteration of the firebase.App and firebase.auth APIs. These enable the user to initialize the SDK (i.e. create an instance of firebase.App) from a certificate credential, and use it to create custom auth tokens, and verify ID tokens signed by Firebase backend servers.
Unit tests (executable via pytest)
Pylint integration for checking code quality
Initial documentation on running unit tests and using pylint

…nstead of two.

jwngr

Looks good so far! I very briefly skimmed the actual source and test files. If I should take a closer look because things changed drastically, please let me know.

jwngr · 2017-03-21T18:57:33Z

.pylintrc

@@ -0,0 +1,410 @@
+[MASTER]


Is it safe to assume this was copied from some Google internal .pylintrc file?

Google's internal configuration has a bunch of Google specific commands and directives that pylint cannot recognize. So I took the default config used by pylint, and slightly modified it to match some of our internal requirements (e.g. disabling reports, line length 80 etc.). Generally speaking this version of .pylintrc is even more stricter than what we use internally. We can use it for a while and relax some of the constraints to fit our needs in the future.

Great, thanks for clarifying.

jwngr · 2017-03-21T18:59:08Z

.pylintrc

+[FORMAT]
+
+# Maximum number of characters on a single line.
+max-line-length=80


How do you feel about this? One of my biggest frustrations with google3 is the very small line lengths. I've been using 120 in the Admin Node.js SDK, but even 100 is better than 80. Now that we aren't in google3, we can tailor this to what we like. If 80 is what you prefer, we can of course keep it.

I'd prefer something higher as well. Lets go with 100. That's what pylint recommends by default. I'll just change the pylint config for now. I'll send another PR with the source files reformatted.

jwngr · 2017-03-21T18:59:55Z

README.md

@@ -1,2 +1,84 @@
 # Firebase Admin Python SDK

+Firebase Admin Python SDK enables server-side (backend) Python developers


Add "The" to the beginning of this sentence.

jwngr · 2017-03-21T19:00:18Z

README.md

@@ -1,2 +1,84 @@
 # Firebase Admin Python SDK

+Firebase Admin Python SDK enables server-side (backend) Python developers
+to integrate [Firebase](http://firebase.google.com) into their services


s/http/https

jwngr · 2017-03-21T19:00:54Z

README.md

+Firebase Admin Python SDK enables server-side (backend) Python developers
+to integrate [Firebase](http://firebase.google.com) into their services
+and applications. Currently this SDK provides Firebase custom authentication
+support. Other Firebase APIs will be added soon.


Let's drop this last sentence. If there's one thing I've learned, it's always better to under-promise and over-deliver :)

jwngr · 2017-03-21T19:07:24Z

README.md

+
+```
+./lint.sh
+./lint.sh all


Suggestion to aide those who skim docs:

./lint.sh # Lint locally modified source files ./lint.sh all # Lint all source files

jwngr · 2017-03-21T19:10:54Z

README.md

@@ -1,2 +1,84 @@
 # Firebase Admin Python SDK


There is some great info here! I think in the long-term, we are going to want to have the README be more of a short description of what the project is, how to install and use it, link to the release notes, and links to the relevant documentation. The info on running tests and linting will move in the .github/CONTRIBUTION.md file since most people wont' need that info up front. This is how the Admin Node.js SDK and a lot of our existing open source projects are structured. This can be done in a follow-up PR though and we probably should come up with a common template of sorts to share amongst the Admin SDK repos.

jwngr · 2017-03-21T19:11:34Z

firebase/__init__.py

@@ -0,0 +1,157 @@
+"""Firebase Admin SDK for Python."""


I'm not reviewing the source and test files that closely since I assume they are mostly copy-pasted. If there are important differences, please call them out for me.

Sources haven't changed at all. But I've reimplemented the tests using pytest (as opposed to unittest). Pytest is a lot more flexible and powerful. Some of the other open source libraries (e.g. oauth2client) have also switched to it.

With pytest our test sources became shorter by about 100 lines, while giving us more test cases than we had :)

Awesome! I gave the tests a closer look then since the code changed in a non-trivial way.

jwngr · 2017-03-21T19:13:34Z

tests/data/private_key.pem

@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----


Are we okay checking this private key into the repo? This will be public, so we need to make sure this doesn't actually give anyone access to anything. Ideally we can just use mocked private keys, which is what I did in the Admin Node.js SDK.

I replaced this with the private key at https://github.com/firebase/firebase-admin-node/blob/master/test/resources/mock.key.json

Had to generate a self-signed cert for it to update public_certs.json

jwngr · 2017-03-21T19:13:56Z

tests/data/service_account.json

@@ -0,0 +1,12 @@
+{


Ditto on my message above about the private key.

Used the mock private key as above.

Great, thanks! I know this is a bit picky, but can we also switch out project_id, client_email, client_id, and client_x509_cert_url with clearly mock values (e.g. "mock-project-id", "mock-email@mock-project-id.iam.gserviceaccount.com", "1234567890", and "https://www.googleapis.com/robot/v1/metadata/x509/mock-emai%40mock-project-id.iam.gserviceaccount.com")?

… private key from Node SDK tests for testing

hiranya911 · 2017-03-22T00:49:32Z

Made the suggested changes. Please take a look at the test sources, since they underwent some changes during migration to pytest.

jwngr

Awesome, I'm really happy with how this is coming along! I took a deeper dive into the new tests and had some cleanup suggestions. So, back to you for another round.

jwngr · 2017-03-22T05:14:49Z

.pylintrc

@@ -0,0 +1,410 @@
+[MASTER]


Great, thanks for clarifying.

jwngr · 2017-03-22T05:15:57Z

firebase/__init__.py

@@ -0,0 +1,157 @@
+"""Firebase Admin SDK for Python."""


Awesome! I gave the tests a closer look then since the code changed in a non-trivial way.

jwngr · 2017-03-22T05:18:25Z

tests/data/service_account.json

@@ -0,0 +1,12 @@
+{


Great, thanks! I know this is a bit picky, but can we also switch out project_id, client_email, client_id, and client_x509_cert_url with clearly mock values (e.g. "mock-project-id", "mock-email@mock-project-id.iam.gserviceaccount.com", "1234567890", and "https://www.googleapis.com/robot/v1/metadata/x509/mock-emai%40mock-project-id.iam.gserviceaccount.com")?

jwngr · 2017-03-22T05:20:59Z

tests/test_app.py

+    @pytest.mark.parametrize('name', invalid_names)
+    def test_app_get_with_invalid_name(self, name):
+        with pytest.raises(ValueError):
+            firebase.initialize_app(OPTIONS, name)


I think this should be get_app(), not initialize_app().

jwngr · 2017-03-22T05:22:42Z

tests/test_app.py

+
+
+class TestFirebaseApp(object):
+    """Test cases for App initialization and life cycle."""


Looks like we don't have tests for delete_app(). Can you add them in this PR or put together a follow-up PR for it? I know the API might be changing according to our API proposal, so it's fine to push this to a later PR to avoid throw-away work.

Sounds good. I'll add it in a future PR. I will have to send another PR anyway to implement the changes proposed in the API proposal.

jwngr · 2017-03-22T05:31:06Z

tests/test_auth.py

+        'kid': 'd98d290613ae1468f7e5f5cf604ead38ca9c8358'
+    }
+    payload = {
+        'aud': 'mg-test-1210',


Can we pull these dynamically from tests/data/service_account.json?

Done. Replaced with CREDENTIAL.project_id

jwngr · 2017-03-22T05:33:35Z

tests/test_auth.py

+    return AuthFixture(request.param)
+
+@pytest.fixture
+def non_cert_app():


I can't seem to wrap my head around how this works. Can you add a comment explaining what this is?

Added some docstrings explaining what's going on here.

jwngr · 2017-03-22T05:35:40Z

tests/test_auth.py

+        id_token = get_id_token()
+        gcloud_project = os.environ.get(auth.GCLOUD_PROJECT_ENV_VAR)
+        try:
+            os.environ[auth.GCLOUD_PROJECT_ENV_VAR] = 'mg-test-1210'


Pull project ID dynamically.

jwngr · 2017-03-22T05:37:23Z

tests/test_auth.py

+    def test_no_project_id(self, non_cert_app):
+        id_token = get_id_token()
+        gcloud_project = None
+        if os.environ.has_key(auth.GCLOUD_PROJECT_ENV_VAR):


I think this block could be slightly cleaned up:

gcloud_project = os.environ.get(auth.GCLOUD_PROJECT_ENV_VAR) if gcloud_project: del os.environ[auth.GCLOUD_PROJECT_ENV_VAR]

jwngr · 2017-03-22T05:40:29Z

tests/test_auth.py

+        'iat': int(time.time()) - 100,
+        'exp': int(time.time()) + 3600,
+        'sub': '1234567890',
+        'uid': USER,


Technically ID tokens do not have a uid claim, so I think we should drop it in this test fixture. It also means we need to add code in the future to explicitly copy the sub claim to the uid claim and add a test to ensure this happens.

Done. Removed the uid claim, and updated the affected test cases to not look for it. We will update those test cases once we implement this claim copy functionality.

…ements in PR

hiranya911 · 2017-03-22T21:00:41Z

All comments addressed

jwngr

LGTM. Back to you to merge. Let's see if the Jenkins job to sync repos works...

Hiranya Jayathilaka added 7 commits March 17, 2017 13:27

Initial code contribution with auth API implementation and unit tests

1fc8187

Added linter integration

b0a21b9

Added link to pylint docs

6d05c42

Improved linter script and documentation

b5bf2d0

Updated tests to use pytest; Updated lint.sh to use one config file i…

15641ad

…nstead of two.

Updated documentation

73e4814

Updated documentation

1d9b5a2

hiranya911 requested a review from jwngr March 20, 2017 20:58

hiranya911 assigned jwngr Mar 20, 2017

jwngr suggested changes Mar 21, 2017

View reviewed changes

jwngr assigned hiranya911 and unassigned jwngr Mar 21, 2017

Changed max line length to 100; Updated documentation; Using the mock…

66993ff

… private key from Node SDK tests for testing

hiranya911 assigned jwngr and unassigned hiranya911 Mar 22, 2017

jwngr suggested changes Mar 22, 2017

View reviewed changes

jwngr assigned hiranya911 and unassigned jwngr Mar 22, 2017

Hiranya Jayathilaka added 2 commits March 22, 2017 13:47

Changing mock crypto field names; Implementing other suggested improv…

3ce270c

…ements in PR

Reformatted file to get rid of unnecessary whitespace characters

aa80ca9

hiranya911 assigned jwngr and unassigned hiranya911 Mar 22, 2017

jwngr approved these changes Mar 22, 2017

View reviewed changes

jwngr assigned hiranya911 and unassigned jwngr Mar 22, 2017

hiranya911 merged commit 9c60c17 into master Mar 22, 2017

hiranya911 deleted the hkj-auth-api branch March 23, 2017 20:38

		@@ -1,2 +1,84 @@
		# Firebase Admin Python SDK

		Firebase Admin Python SDK enables server-side (backend) Python developers



		class TestFirebaseApp(object):
		"""Test cases for App initialization and life cycle."""

Implementing Firebase SDK Initialization and Custom Authentication #1

Implementing Firebase SDK Initialization and Custom Authentication #1

Uh oh!

Conversation

hiranya911 commented Mar 20, 2017

Uh oh!

jwngr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 commented Mar 22, 2017

Uh oh!

jwngr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Mar 21, 2017 •

edited

Loading